backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)

208-231: Consider memory implications for large datasets.

Loading all chapters into memory at once (line 217) could be problematic if there are many active chapters. For more resilient processing, consider either:

1. Processing in batches
2. Using .iterator() to stream results
3. Adding progress indicators for long-running operations

Example with error handling and progress:

if entity_type in ["chapter", "both"]:
    chapter_queryset = Chapter.objects.filter(is_active=True)

    if key:
        chapter_queryset = chapter_queryset.filter(key=key)

    if offset:
        chapter_queryset = chapter_queryset[offset:]

    total_chapters = chapter_queryset.count()
    self.stdout.write(f"Processing {total_chapters} chapters...")

    chapters_to_update = []
    for idx, chapter in enumerate(chapter_queryset.iterator(), 1):
        try:
            contribution_data = self.aggregate_chapter_contributions(
                chapter,
                start_date,
            )
            chapter.contribution_data = contribution_data
            chapters_to_update.append(chapter)
            
            if idx % 100 == 0:
                self.stdout.write(f"  Progress: {idx}/{total_chapters}")
        except Exception as e:
            self.stdout.write(
                self.style.WARNING(f"  Failed to aggregate chapter {chapter.key}: {e}")
            )

    if chapters_to_update:
        Chapter.bulk_save(chapters_to_update, fields=("contribution_data",))
        self.stdout.write(
            self.style.SUCCESS(f"✓ Updated {len(chapters_to_update)} chapters"),
        )

---

234-257: Apply the same memory and error handling improvements for projects.

The same concerns about memory usage and error handling apply here as in the chapter processing section.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (4)

49-67: Consider database-level aggregation for better performance.

The current implementation fetches all date values into memory and aggregates in Python. For repositories with many contributions, this could be inefficient.

Consider using Django's aggregation framework to let the database do the grouping:

from django.db.models import Count
from django.db.models.functions import TruncDate

def _aggregate_contribution_dates(
    self,
    queryset,
    date_field: str,
    contribution_map: dict[str, int],
) -> None:
    """Aggregate contribution dates from a queryset into the contribution map."""
    # Use database aggregation instead of fetching all values
    aggregated = (
        queryset
        .annotate(date_key=TruncDate(date_field))
        .values('date_key')
        .annotate(count=Count('id'))
    )
    
    for item in aggregated:
        if item['date_key']:
            date_key = item['date_key'].isoformat()
            contribution_map[date_key] = contribution_map.get(date_key, 0) + item['count']

This approach:

• Performs grouping at the database level (more efficient)
• Reduces memory usage by fetching only aggregated counts
• Scales better for large datasets

---

218-242: Add prefetch_related to reduce database queries.

The current implementation accesses chapter.owasp_repository for each chapter in the loop (line 232-236), which can cause N+1 query issues.

Apply this optimization:

         # Process chapters
         if entity_type in ["chapter", "both"]:
-            chapter_queryset = Chapter.objects.filter(is_active=True)
+            chapter_queryset = Chapter.objects.filter(is_active=True).select_related('owasp_repository')
 
             if key:
                 chapter_queryset = chapter_queryset.filter(key=key)

This prefetches the repository relationship in a single query, avoiding N additional queries when iterating through chapters.

---

244-268: Add prefetch_related for project repositories.

Similar to chapters, accessing project.repositories.all() and project.owasp_repository inside the loop causes N+1 queries.

Apply this optimization:

         # Process projects
         if entity_type in ["project", "both"]:
-            project_queryset = Project.objects.filter(is_active=True)
+            project_queryset = Project.objects.filter(is_active=True).select_related(
+                'owasp_repository'
+            ).prefetch_related('repositories')
 
             if key:
                 project_queryset = project_queryset.filter(key=key)

This prefetches both the owasp_repository (one-to-one) and repositories (many-to-many) relationships efficiently.

---

31-36: Consider validating the days parameter.

The --days parameter accepts any integer, including negative values or extremely large values that could cause issues.

Add validation in the handle method:

def handle(self, *args, **options):
    """Execute the command."""
    entity_type = options["entity_type"]
    days = options["days"]
    
    # Validate days parameter
    if days < 1:
        self.stdout.write(
            self.style.ERROR("Error: --days must be a positive integer")
        )
        return
    
    if days > 3650:  # 10 years
        self.stdout.write(
            self.style.WARNING(
                f"Warning: --days is set to {days}, which may cause performance issues"
            )
        )
    
    # ... rest of the method

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)

16-41: Consider adding a dedicated test method for contribution_data type.

Following the pattern of other fields in this test file (e.g., test_resolve_issues_count, test_resolve_key), consider adding a dedicated test to verify the contribution_data field type. This would strengthen test coverage for this core PR feature and align with the PR objectives to "Add tests covering resolvers and data formatting."

Example test to add after line 106:

def test_resolve_contribution_data(self):
    field = self._get_field_by_name("contribution_data")
    assert field is not None
    # Verify the type matches the expected JSON/dict type used in your GraphQL schema
    # For Strawberry GraphQL with JSONScalar, this might be:
    # assert field.type is strawberry.scalars.JSON
    # Or check for dict if using a dict type annotation

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (4)

161-173: Use modern mock assertion methods.

The pattern assert mock_bulk_save.called is deprecated. Use mock_bulk_save.assert_called() or mock_bulk_save.assert_called_once() for clearer intent and better error messages.

Apply this diff:

-        assert mock_bulk_save.called
+        mock_bulk_save.assert_called_once()

---

177-189: Use modern mock assertion methods.

Same as above—prefer mock_bulk_save.assert_called_once() over the deprecated assert mock_bulk_save.called pattern.

Apply this diff:

-        assert mock_bulk_save.called
+        mock_bulk_save.assert_called_once()

---

195-224: Use modern mock assertion methods.

Both assertions should use assert_called_once() instead of the deprecated .called attribute check.

Apply this diff:

-        assert mock_chapter_bulk_save.called
-        assert mock_project_bulk_save.called
+        mock_chapter_bulk_save.assert_called_once()
+        mock_project_bulk_save.assert_called_once()

---

227-242: Strengthen filter argument verification.

The test verifies the filter was called but doesn't check that the key parameter was actually used. Consider verifying the filter arguments to ensure the key filtering logic works correctly.

You could strengthen the assertion like this:

# Verify filter was called with the specific key
call_args = mock_filter.call_args
# Verify 'key' in the filter kwargs or positional Q object

Or use assert_called_with() if the filter signature is predictable:

mock_filter.assert_called_with(key="www-chapter-test")

backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)

15-32: Consider adding type validation tests for remaining fields.

The test validates that created_at, name, and summary fields exist, but unlike other fields (lines 39-82), they lack dedicated type validation tests.

For completeness, add type validation tests for the remaining fields:

def test_resolve_created_at(self):
    field = self._get_field_by_name("created_at")
    assert field is not None
    # Verify expected datetime type
    from datetime import datetime
    # Adjust based on how Strawberry exposes datetime fields

def test_resolve_name(self):
    field = self._get_field_by_name("name")
    assert field is not None
    assert field.type is str

def test_resolve_summary(self):
    field = self._get_field_by_name("summary")
    assert field is not None
    assert field.type is str

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)

48-66: Consider using .iterator() to reduce memory usage for large datasets.

For repositories with many thousands of contributions, values_list(..., flat=True) loads all dates into memory at once. This can be memory-intensive and slow.

Apply this diff to stream results:

     def _aggregate_contribution_dates(
         self,
         queryset,
         date_field: str,
         contribution_map: dict[str, int],
     ) -> None:
         """Aggregate contribution dates from a queryset into the contribution map.
 
         Args:
             queryset: Django queryset to aggregate
             date_field: Name of the date field to aggregate on
             contribution_map: Dictionary to update with counts
 
         """
-        dates = queryset.values_list(date_field, flat=True)
-        for date_value in dates:
+        for date_value in queryset.values_list(date_field, flat=True).iterator():
             if date_value:
                 date_key = date_value.date().isoformat()
                 contribution_map[date_key] = contribution_map.get(date_key, 0) + 1

---

202-269: Consider wrapping bulk operations in transaction.atomic().

The bulk save operations for chapters (Line 238) and projects (Line 264) are not wrapped in transactions. If an error occurs during bulk_save, you may end up with partial updates. Wrapping each entity type's processing in transaction.atomic() ensures all-or-nothing updates.

Example for chapter processing:

from django.db import transaction

# Inside handle method
if entity_type in ["chapter", "both"]:
    with transaction.atomic():
        chapter_queryset = Chapter.objects.filter(is_active=True).select_related(
            "owasp_repository"
        )
        # ... rest of chapter processing
        if chapters:
            Chapter.bulk_save(chapters, fields=("contribution_data",))

Apply the same pattern to project processing.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

frontend/src/app/projects/[projectKey]/page.tsx (1)

93-98: Duplicate of earlier refactoring suggestion.

This date range calculation is identical to the one in the chapter page. See the earlier comment on lines 64-69 of frontend/src/app/chapters/[chapterKey]/page.tsx for the recommended refactoring to extract this into a shared utility function.

backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (2)

34-52: Verify field nullability to prevent test failures with Optional-wrapped types.

The identity checks (field.type is str, field.type is bool) will fail at runtime if Strawberry wraps any of these fields in Optional due to null=True in the model definition. The past review raised this concern for owasp_repository, but the same pattern persists here for key, country, region, and is_active.

Run this script to check if any of these fields are nullable in the Chapter model:

#!/bin/bash
# Check nullability of fields tested with identity checks

ast-grep --pattern $'class Chapter($$$):
  $$$
  key = $$$
  $$$'

ast-grep --pattern $'class RepositoryBasedEntityModel($$$):
  $$$
  country = $$$
  $$$
  region = $$$
  $$$
  is_active = $$$
  $$$'

If any field has null=True, update the corresponding test to handle the Optional wrapper using typing.get_origin() to extract the inner type, as shown in the past review comment for owasp_repository.

---

54-57: Complete the type assertion for contribution_data.

The test verifies field existence but omits type verification, unlike all other field tests in this class. The comment indicates it should be a JSON scalar type, but this isn't asserted.

Apply this diff to add the missing assertion:

+from strawberry.scalars import JSON
+
 def test_resolve_contribution_data(self):
     field = self._get_field_by_name("contribution_data")
     assert field is not None
-    # contribution_data is a JSON scalar type
+    assert field.type is JSON

frontend/src/app/chapters/[chapterKey]/page.tsx (1)

64-69: Extract date range calculation into a shared utility function.

This date range calculation logic is duplicated identically in the project page (frontend/src/app/projects/[projectKey]/page.tsx, lines 93-98). Extract it into a reusable utility function to follow DRY principles.

Create a utility function in utils/dateFormatter.ts or a new utils/contributionHelpers.ts:

export function getContributionDateRange(): { startDate: string; endDate: string } {
  const today = new Date()
  const oneYearAgo = new Date(today)
  oneYearAgo.setFullYear(today.getFullYear() - 1)
  return {
    startDate: oneYearAgo.toISOString().split('T')[0],
    endDate: today.toISOString().split('T')[0],
  }
}

Then use it in both files:

-  const today = new Date()
-  const oneYearAgo = new Date(today)
-  oneYearAgo.setFullYear(today.getFullYear() - 1)
-  const startDate = oneYearAgo.toISOString().split('T')[0]
-  const endDate = today.toISOString().split('T')[0]
+  const { startDate, endDate } = getContributionDateRange()

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)

18-18: Consider adding docstrings to magic methods (optional).

While not critical for a test helper class, adding brief docstrings to __iter__, __getitem__, and __len__ would improve code documentation.

As per static analysis hints.

Also applies to: 21-21, 30-30

---

258-258: Strengthen the assertion to verify filter arguments.

The test only checks that filter was called but doesn't verify it was called with the specific key. Consider using assert_called_with or inspecting call_args to ensure the key parameter is properly passed to the filter.

Example:

-        mock_chapter_model.objects.filter.assert_called()
+        # Verify filter was called with the specific key
+        call_args = mock_chapter_model.objects.filter.call_args
+        assert call_args[1].get('key') == "www-chapter-test" or 'key' in str(call_args)

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)

217-267: Consider adding select_related / prefetch_related to avoid N+1 queries in handle.

handle() currently builds chapters = list(Chapter.objects.filter(is_active=True)...) and projects = list(Project.objects.filter(is_active=True)...) without select_related("owasp_repository") or prefetch_related("repositories"). Since aggregate_chapter_contributions and aggregate_project_contributions touch these relations for each entity, this will trigger extra queries per chapter/project.

For a scheduled aggregation over many entities, consider:

• For chapters: .select_related("owasp_repository").
• For projects: .select_related("owasp_repository").prefetch_related("repositories").

This keeps behavior identical while improving performance during bulk runs.

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)

245-295: Complete the handle scenario assertions (especially offset).

test_handle_with_offset still ends with a comment but does not assert the behavior it describes, so regressions in offset handling would go unnoticed. Similarly, test_handle_with_specific_key only checks that .objects.filter was called, not that the key filtering path was exercised.

Consider tightening these tests, for example:

@@ def test_handle_with_offset(...):
-        with mock.patch.object(
-            command,
-            "aggregate_chapter_contributions",
-            return_value={"2024-11-16": 1},
-        ):
-            command.handle(entity_type="chapter", offset=2, days=365)
-
-        # Verify that offset was applied - only 1 chapter should be processed (3 total - 2 offset)
+        with mock.patch.object(
+            command,
+            "aggregate_chapter_contributions",
+            return_value={"2024-11-16": 1},
+        ) as mock_aggregate:
+            command.handle(entity_type="chapter", offset=2, days=365)
+
+        # 3 total chapters, offset=2 -> only 1 should be processed
+        assert mock_aggregate.call_count == 1
+        mock_chapter_model.bulk_save.assert_called_once()
@@ def test_handle_with_specific_key(...):
-        # Verify filter was called with the specific key
-        mock_chapter_model.objects.filter.assert_called()
+        # Verify initial active filter and that we attempted to narrow by key
+        mock_chapter_model.objects.filter.assert_called_with(is_active=True)
+        # Optionally, track calls on the queryset.filter(...) as well if you want

This keeps the tests aligned with their comments and makes the control flow around offset and key much harder to accidentally break.

frontend/src/app/projects/[projectKey]/page.tsx (1)

93-134: Heatmap integration looks solid; consider simplifying the wrapper styling.

The 1‑year date range calculation and conditional contributionData guard are correct and align with the backend aggregation. The only minor concern is the min-h-screen + explicit background wrapper around the heatmap, which may introduce a very tall section and override the page background differently from the DetailsCard section. Consider dropping min-h-screen (or reusing the layout pattern from existing heatmap usages) if you want more consistent page flow and styling.

frontend/src/app/chapters/[chapterKey]/page.tsx (1)

64-98: Consistent chapter heatmap wiring; same layout consideration as projects page.

The contribution date range and conditional rendering are correct and mirror the project page, which is good for consistency. As with the project view, the min-h-screen + background wrapper around the heatmap can create an extra-tall, differently styled block; consider dropping min-h-screen or aligning the container styling with nearby content for a smoother layout.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)

260-275: Complete the test by adding assertions.

The test has a comment stating "Verify that offset was applied" but lacks any assertions to actually verify this behavior. This issue was previously flagged but remains unresolved. The test is incomplete and will pass regardless of whether the offset logic works correctly.

Apply this diff to add the necessary assertions:

         with mock.patch.object(
             command,
             "aggregate_chapter_contributions",
             return_value={"2024-11-16": 1},
-        ):
+        ) as mock_aggregate:
             command.handle(entity_type="chapter", offset=2, days=365)
 
         # Verify that offset was applied - only 1 chapter should be processed (3 total - 2 offset)
+        assert mock_aggregate.call_count == 1, \
+            "Expected aggregate to be called once for 1 remaining chapter after offset"
+        mock_chapter_model.bulk_save.assert_called_once()

Based on past review comments

frontend/src/components/ContributionHeatmap.tsx (2)

13-13: Remove unused title prop.

The title prop is defined in the interface but is never used in the component. This appears to be leftover from a refactor where title rendering was removed (around lines 114-182 in the old version).

Apply this diff:

 interface ContributionHeatmapProps {
   contributionData: Record<string, number>
   startDate: string
   endDate: string
-  title?: string
   unit?: string
   stats?: {

---

15-20: Remove unused stats prop.

The stats prop is defined but never referenced in the component body. If this was intended for future use or display, it should either be implemented or removed to avoid confusion.

Apply this diff to remove the unused prop:

 interface ContributionHeatmapProps {
   contributionData: Record<string, number>
   startDate: string
   endDate: string
   unit?: string
-  stats?: {
-    commits?: number
-    pullRequests?: number
-    issues?: number
-    total?: number
-  }
   variant?: 'default' | 'compact'
 }

frontend/src/app/chapters/[chapterKey]/page.tsx (1)

158-169: Incorrect icon choice for "Total" metric.

The faCodeMerge icon semantically represents merge/pull request operations, not total contributions. Consider using a more appropriate icon like faChartLine, faChartBar, or faCalculator to represent an aggregate total.

Apply this diff:

+import { faChartLine } from '@fortawesome/free-solid-svg-icons'
 ...
                 <div className="flex items-center gap-2">
                   <FontAwesomeIcon
-                    icon={faCodeMerge}
+                    icon={faChartLine}
                     className="h-4 w-4 text-gray-600 dark:text-gray-400"
                   />

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)

26-28: Consider documenting the filter method's mock behavior.

The filter method returns self without applying any actual filtering logic. While this is acceptable for simple test mocking, it could lead to misleading test results if tests inadvertently rely on filtering behavior. Consider adding a docstring or comment to make this limitation explicit.

Apply this diff to clarify the behavior:

     def filter(self, **kwargs):
-        # Return self to support filter chaining
+        # Mock filter: returns self without applying actual filtering logic.
+        # Tests should not depend on filter criteria being evaluated.
         return self

---

244-259: Strengthen assertion to verify filter arguments.

The test verifies that filter was called but doesn't confirm it was called with the correct key argument. Consider using assert_called_with to verify the exact filter criteria.

Apply this diff to strengthen the assertion:

-        # Verify filter was called with the specific key
-        mock_chapter_model.objects.filter.assert_called()
+        # Verify filter was called with the specific key
+        calls = mock_chapter_model.objects.filter.call_args_list
+        assert any(call.kwargs.get('key') == 'www-chapter-test' for call in calls), \
+            "Expected filter to be called with key='www-chapter-test'"

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)

245-255: Avoid N+1 queries for projects by prefetching repositories and selecting owasp_repository

aggregate_project_contributions calls project.repositories.all() and accesses project.owasp_repository for each project. Because project_queryset does not currently use prefetch_related / select_related, this results in N+1 queries as the number of projects grows.

Optimize the queryset before evaluating it:

-        if entity_type in ["project", "both"]:
-            project_queryset = Project.objects.filter(is_active=True)
-
-            if key:
-                project_queryset = project_queryset.filter(key=key)
-
-            if offset:
-                project_queryset = project_queryset[offset:]
-
-            projects = list(project_queryset)
+        if entity_type in ["project", "both"]:
+            project_queryset = Project.objects.filter(is_active=True)
+
+            if key:
+                project_queryset = project_queryset.filter(key=key)
+
+            if offset:
+                project_queryset = project_queryset[offset:]
+
+            project_queryset = project_queryset.prefetch_related(
+                "repositories",
+            ).select_related("owasp_repository")
+
+            projects = list(project_queryset)

This keeps existing filtering/offset semantics while collapsing the per‑project relationship lookups into a small, fixed number of queries.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)

48-67: Make _aggregate_contribution_dates resilient to date/datetime variants

date_value.date().isoformat() assumes a datetime instance. If this helper is ever reused with a DateField (or any non-datetime type), it will raise at runtime. You could defensively handle both date and datetime here to future‑proof the helper without changing current behavior.

Example:

-        for date_value in dates:
-            if date_value:
-                date_key = date_value.date().isoformat()
-                contribution_map[date_key] = contribution_map.get(date_key, 0) + 1
+        for date_value in dates:
+            if not date_value:
+                continue
+
+            if hasattr(date_value, "date"):
+                date_obj = date_value.date()
+            else:
+                date_obj = date_value
+
+            date_key = date_obj.isoformat()
+            contribution_map[date_key] = contribution_map.get(date_key, 0) + 1

This keeps current semantics for DateTimeField and won’t break if a future caller passes a plain date.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)

12-32: Consider adding docstrings to magic methods for better maintainability.

The MockQuerySet class provides a clear purpose, but the magic methods (__iter__, __getitem__, __len__) lack docstrings. While this is a test helper, adding brief docstrings would improve maintainability.

Example:

 def __iter__(self):
+    """Iterate over items."""
     return iter(self._items)

 def __getitem__(self, key):
+    """Support index and slice access."""
     if isinstance(key, slice):
         return MockQuerySet(self._items[key])
     return self._items[key]

 def __len__(self):
+    """Return number of items."""
     return len(self._items)

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

frontend/src/app/projects/[projectKey]/page.tsx (1)

107-125: Backend should provide per-type contribution data instead of frontend estimation.

The frontend uses arbitrary percentages (60% commits, 23% issues, 15% PRs) to estimate contribution breakdown from the aggregated total. This produces inaccurate statistics that don't reflect the actual contribution distribution.

As noted in the previous review, the backend aggregation logic should store per-type daily counts instead of a single aggregated count, enabling accurate statistics without estimation.

frontend/src/app/projects/[projectKey]/page.tsx (1)

159-208: Consider more semantically appropriate icons.

The icon choices are functional but could be refined:

• Line 174: faCodeBranch for PRs is acceptable but not ideal—branches are a precursor to PRs rather than representing the PR itself.
• Line 198: faCodeMerge for "Total" is semantically incorrect—merge represents a specific action, not an aggregate count. Consider faChartLine or a more generic aggregation icon.

These are minor clarity improvements and don't affect functionality.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

frontend/src/app/projects/[projectKey]/page.tsx (1)

105-125: Frontend estimation of contribution breakdown remains inaccurate.

This is the same issue identified in the previous review: the hardcoded percentages (60% commits, 23% issues, 15% PRs) produce inaccurate statistics that don't reflect actual contribution distribution.

The backend should store per-type contribution counts instead of aggregated totals. Refer to the previous review comment for the recommended solution.

backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)

54-58: Consider using isinstance or direct type comparison for more robust type checking.

The test verifies the field type by comparing __class__.__name__ to the string "ScalarWrapper", which is fragile and will break if the class is renamed or if the import path changes.

Consider a more robust approach:

 def test_resolve_contribution_data(self):
     field = self._get_field_by_name("contribution_data")
     assert field is not None
-    # JSONField is represented as a Strawberry ScalarWrapper for JSON type
-    assert field.type.__class__.__name__ == "ScalarWrapper"
+    # JSONField is represented as a Strawberry ScalarWrapper for JSON type
+    from strawberry.types.types import StrawberryType
+    assert isinstance(field.type, StrawberryType) or hasattr(field.type, '__strawberry_definition__')

Alternatively, if you need to check specifically for JSON scalar, import the actual type:

# At top of file
from strawberry.scalars import JSON

def test_resolve_contribution_data(self):
    field = self._get_field_by_name("contribution_data")
    assert field is not None
    # Check if it's the JSON scalar type or wrapped version
    assert field.type is JSON or (hasattr(field.type, 'of_type') and field.type.of_type is JSON)

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

frontend/src/app/projects/[projectKey]/page.tsx (1)

101-121: Same hardcoded percentage estimation issue as chapter page.

This code uses the same arbitrary percentage breakdown (60%, 23%, 15%) to estimate contribution statistics. The root cause is identical: the backend aggregates all contribution types into single daily counts, forcing frontend estimation.

Apply the same fix as suggested for the chapter page (lines 72-92 in frontend/src/app/chapters/[chapterKey]/page.tsx):

1. Modify backend to store per-type daily counts
2. Update frontend to sum actual per-type values
3. Coordinate changes across backend models, GraphQL, and frontend types

frontend/src/components/ContributionHeatmap.tsx (1)

167-172: Consider using date-fns for more robust date parsing.

The string concatenation (date + 'T00:00:00Z') works but is fragile if the date format changes. Since date-fns is already a project dependency, consider using parseISO for safer parsing.

Apply this diff:

+import { parseISO, format } from 'date-fns'
+import { formatInTimeZone } from 'date-fns-tz'

       const count = data.y
       const date = data.date
-      // Parse date as UTC to match data format
-      const formattedDate = new Date(date + 'T00:00:00Z').toLocaleDateString('en-US', {
-        weekday: 'short',
-        month: 'short',
-        day: 'numeric',
-        timeZone: 'UTC',
-      })
+      // Parse ISO date and format in UTC
+      const formattedDate = formatInTimeZone(
+        parseISO(date),
+        'UTC',
+        'EEE, MMM d'
+      )

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

frontend/src/components/ContributionStats.tsx (2)

67-78: Consider a more semantically appropriate icon for the Total metric.

The faCodeMerge icon specifically represents code/PR merging operations, which may confuse users since "Total" represents the sum of all contribution types, not just merges. Consider alternatives like faChartBar, faCalculator, or faListOl that better convey aggregation or totals.

Apply this diff to use a more appropriate icon:

 import {
   faChartLine,
   faCode,
   faCodeBranch,
-  faCodeMerge,
+  faChartBar,
   faExclamationCircle,
 } from '@fortawesome/free-solid-svg-icons'

Then update the icon reference:

         <div className="flex items-center gap-2">
           <FontAwesomeIcon
-            icon={faCodeMerge}
+            icon={faChartBar}
             className="h-5 w-5 text-gray-600 dark:text-gray-400"
           />

---

33-79: Consider adding accessibility attributes for screen readers.

The statistics grid lacks semantic structure and ARIA labels. While the visual presentation is clear, screen reader users would benefit from explicit role and label attributes.

Apply this diff to improve accessibility:

-      <div className="mb-6 grid grid-cols-2 gap-4 sm:grid-cols-4">
+      <div className="mb-6 grid grid-cols-2 gap-4 sm:grid-cols-4" role="list" aria-label="Contribution statistics">
-        <div className="flex items-center gap-2">
+        <div className="flex items-center gap-2" role="listitem">
           <FontAwesomeIcon icon={faCode} className="h-5 w-5 text-gray-600 dark:text-gray-400" />
-          <div>
+          <div aria-label={`${formatNumber(stats?.commits)} commits`}>
             <p className="text-sm font-medium text-gray-500 dark:text-gray-400">Commits</p>

Apply similar changes to the other three metric blocks (PRs, Issues, Total).

frontend/src/components/ContributionHeatmap.tsx (2)

263-288: Consider refactoring dynamic CSS generation for maintainability.

The current approach uses template strings to generate CSS classes and applies transforms via inline styles. While functional, this approach has some drawbacks:

• Dynamic class names make it harder to debug and inspect styles
• Scaling transforms on the canvas can cause subpixel rendering issues
• The fixed 1200px width on default variant relies on scaling rather than proper responsive layout

Consider:

1. Define static CSS classes for each variant in a CSS module or styled component
2. Use CSS Grid or Flexbox with proper responsive breakpoints instead of transform scaling
3. Let ApexCharts handle responsive sizing via the responsive options array

This would improve maintainability and avoid potential rendering artifacts from CSS transforms.

---

289-297: Add accessibility attributes for the heatmap chart.

The ApexCharts Chart component doesn't support ARIA attributes as direct props. To make the heatmap accessible to screen readers, wrap it in a container with appropriate ARIA attributes.

Apply this diff to add accessibility:

-        <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}>
+        <div 
+          className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}
+          role="img"
+          aria-label={`${title || 'Contribution'} heatmap showing activity from ${new Date(startDate).toLocaleDateString()} to ${new Date(endDate).toLocaleDateString()}`}
+        >
           <Chart

Based on learnings

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

frontend/src/app/chapters/[chapterKey]/page.tsx (1)

72-92: Hardcoded contribution-type percentages still produce misleading stats

The breakdown for commits/issues/PRs is derived from fixed 60%/23%/15% splits of the total, even though the backend only exposes an undifferentiated per-day count. This makes the per-type numbers look precise when they are actually guesses and they may not even sum to the displayed total.

Consider either:

• Dropping the per-type breakdown for now and having ContributionStats focus on totals (e.g., total contributions, active days) until the backend stores per-type counts, or
• Extending the aggregation to persist per-type data (e.g., {date: {commits, issues, pullRequests, releases}}) and updating this logic to sum real values instead of estimating.

Also applies to: 112-119

backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)

10-58: ChapterNode tests are good; JSON ScalarWrapper check is a bit brittle

The tests nicely verify that contribution_data is exposed on ChapterNode, but test_resolve_contribution_data relies on field.type.__class__.__name__ == "ScalarWrapper", which couples the test to Strawberry’s internal class name and may break on library upgrades even if the JSON mapping is still correct.

If you want this test to be more future-proof, consider asserting against the JSON scalar more generically (for example, by checking for expected serialize/parse_value attributes, or by comparing with the configured JSON scalar type if you expose it) instead of depending on the concrete class name.

backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)

190-270: Aggregation command tests are solid; specific-key test could assert more strongly

The mocking strategy and coverage for helper, chapter/project paths, offset, and custom days are thorough. In test_handle_with_specific_key, though, the final assertion only checks that Chapter.objects.filter was called, not that the queryset’s subsequent .filter(key=...) was invoked, so it doesn’t fully validate the behavior described in the docstring.

You could strengthen this by instrumenting MockQuerySet.filter to record kwargs and asserting that one of the calls includes key="www-chapter-test", or by explicitly asserting on mock_chapter_model.objects.filter.call_args_list to ensure the key-based filter path is exercised.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)

202-281: Command orchestration and query patterns look solid; batching is a future optimization

handle and the _process_chapters / _process_projects helpers:

• Respect --entity-type, --days, --key, and --offset.
• Use timezone.now() - timedelta(days=days) consistently for the lower bound.
• Apply select_related("owasp_repository") and prefetch_related("repositories") before materializing lists, which avoids N+1s.
• Persist only contribution_data via bulk_save, which is efficient.

If you ever expect a very large number of chapters/projects, you might consider processing in smaller batches (e.g., via queryset iterator() + chunked bulk saves) to keep memory bounded, but for current usage this is an optional optimization, not a blocker.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)

48-67: Helper cleanly centralizes daily aggregation; per-type breakdown remains a future enhancement

The _aggregate_contribution_dates helper is concise and reusable, and the use of .values_list(...).date().isoformat() gives a stable YYYY-MM-DD -> count map that fits the current JSONField contract.

One trade-off (also raised earlier) is that this structure only tracks a total count per day, so any UI that wants per-type (commits/issues/PRs/releases) statistics has to approximate from totals. Given the current spec explicitly calls for { "YYYY-MM-DD": count, ... }, this is acceptable, but if you later need accurate per-type metrics you’ll need to evolve contribution_map to a nested {date: {type: count}} shape and propagate that through models, GraphQL, and the frontend.

frontend/src/components/ContributionHeatmap.tsx (1)

179-179: Pluralization logic is English-only.

The simple pluralization (count !== 1 ? ${unit}s : unit) will not work correctly for all English words (e.g., "activity" → "activitys") or for internationalized content. If i18n is planned, consider using a proper pluralization library or passing plural forms as separate props.

backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)

133-200: Project aggregation correctly merges repositories; consider deduping IDs defensively

The project aggregation correctly combines project.repositories and an optional project.owasp_repository, then queries by repository_id__in. That matches the requirement to aggregate across all project repositories.

If there’s any chance project.owasp_repository can also appear in project.repositories, you might want to dedupe IDs before passing them to the __in filter (purely for clarity and to avoid redundant parameters):

-        repository_ids = [repo.id for repo in repositories if repo]
+        repository_ids = list({repo.id for repo in repositories if repo})

Functionally this shouldn’t change results, but it makes the intent explicit.

---

31-35: Align days window with date-based buckets to avoid truncating the first day

start_date is computed as timezone.now() - timedelta(days=days) (a datetime), while aggregation buckets contributions by date_value.date().isoformat(). With this combination, events that occurred exactly days days ago but earlier in that day (e.g., morning of the first day) will be excluded even though they share the same calendar date as the first bucket.

For a date-based heatmap, you may get more intuitive results by aligning the window to whole days, e.g., either:

• Normalize start_date to midnight before filtering, or
• Filter on the date component (created_at__date__gte / published_at__date__gte) instead of raw datetimes.

This isn’t a blocker, but tightening this would make the “last N days” behavior match the calendar dates you expose in contribution_data.

Also applies to: 209-215

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

frontend/src/components/ContributionHeatmap.tsx (3)

152-179: Harden tooltip date parsing against malformed input

The tooltip still builds a Date from date + 'T00:00:00Z'. This assumes a valid YYYY-MM-DD string and can silently produce Invalid Date (or off‑by‑one issues if formats ever change). Consider validating and handling failures explicitly, e.g.:

-      const count = data.y
-      const date = data.date
-      // Parse date as UTC to match data format
-      const formattedDate = new Date(date + 'T00:00:00Z').toLocaleDateString('en-US', {
+      const count = data.y
+      const date = data.date
+      // Parse date as UTC to match data format (YYYY-MM-DD expected)
+      const parsedDate = new Date(`${date}T00:00:00Z`)
+      const formattedDate = Number.isNaN(parsedDate.getTime())
+        ? date
+        : parsedDate.toLocaleDateString('en-US', {
           weekday: 'short',
           month: 'short',
           day: 'numeric',
           timeZone: 'UTC',
-      })
+        })

This keeps behavior the same for valid dates while failing more safely if the input ever drifts.

---

271-283: Fixed 1200px chart width still risks horizontal overflow on mid‑sized screens

The combination of:

• .heatmap-container-default { max-width: 100%; overflow: visible; } and
• width={isCompact ? '100%' : '1200px'}

can still cause horizontal overflow and awkward scaling on tablets / smaller desktops even though the canvas is scaled to 0.85.

A more responsive approach would be to keep the chart width fluid and cap via container:

-            .heatmap-container-${isCompact ? 'compact' : 'default'} {
-              width: 100%;
-              ${isCompact ? 'min-width: 380px;' : 'max-width: 100%; overflow: visible;'}
-            }
+            .heatmap-container-${isCompact ? 'compact' : 'default'} {
+              width: 100%;
+              ${isCompact ? 'min-width: 380px;' : 'max-width: 1200px; overflow: visible; margin: 0 auto;'}
+            }
@@
-            ${isCompact ? '' : '.heatmap-container-default .apexcharts-canvas { transform: scale(0.85); transform-origin: left top; }'}
+            ${isCompact ? '' : '.heatmap-container-default .apexcharts-canvas { transform-origin: left top; }'}
@@
-            height={isCompact ? '100%' : 200}
-            width={isCompact ? '100%' : '1200px'}
+            height={isCompact ? '100%' : 200}
+            width="100%"

This preserves the intended max width while letting the chart shrink gracefully with the viewport.

Also applies to: 292-293

---

287-294: Wrap Chart with an ARIA‑aware container for accessibility

react-apexcharts doesn’t take ARIA props directly, so screen readers currently get no semantic information about this visualization. Per project learnings, wrap the Chart in a div with appropriate ARIA attributes:

-        <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}>
-          <Chart
-            options={options}
-            series={heatmapSeries}
-            type="heatmap"
-            height={isCompact ? '100%' : 200}
-            width={isCompact ? '100%' : '1200px'}
-          />
-        </div>
+        <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}>
+          <div
+            role="img"
+            aria-label={`${title || 'Contribution heatmap'} showing activity over time`}
+          >
+            <Chart
+              options={options}
+              series={heatmapSeries}
+              type="heatmap"
+              height={isCompact ? '100%' : 200}
+              width={isCompact ? '100%' : '1200px'}
+            />
+          </div>
+        </div>

This significantly improves accessibility without changing visual behavior. Based on learnings.

frontend/src/components/ContributionHeatmap.tsx (2)

9-66: Heatmap series generation logic looks solid; minor edge-case thoughts

The per-day loop, week labeling, and Monday‑anchored grid all look correct and should produce a consistent 7×N matrix with proper zero‑filled padding around the actual range. Only small nits:

• If startDate > endDate, you silently return empty series; consider early‑returning or asserting to avoid confusing call‑sites.
• You could add an explicit return type for generateHeatmapSeries for clearer reuse and to catch accidental shape changes in future refactors.

---

63-65: Use [...series].reverse() to maintain compatibility with Next.js 15.5.6 defaults

Array.prototype.toReversed requires Firefox 115+, but Next.js 15.5.6 defaults to Firefox 111+, creating a compatibility gap. The current code will break in Firefox 111–114. Replace with the spread+reverse pattern to remain compatible with the stated browserslist:

const reversedSeries = [...series].reverse()

This preserves immutability, works across all supported browsers, and avoids the need for polyfills.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.

frontend/src/components/ContributionHeatmap.tsx (2)

271-283: Dynamic CSS class generation may cause style conflicts if variant changes.

The component generates CSS rules dynamically based on isCompact, but if the variant prop changes after initial render, the old CSS rules remain in the DOM while new ones are added. This creates duplicate style blocks and potential specificity conflicts.

Consider one of these approaches:

Option 1: Use stable class names and CSS variables:

<style>
  {`
    .heatmap-container {
      width: 100%;
    }
    .heatmap-container.compact {
      min-width: 380px;
    }
    .heatmap-container.default {
      max-width: 100%;
      overflow: visible;
    }
    /* ... rest of styles with stable classes */
  `}
</style>
<div className={`heatmap-container ${variant}`}>

Option 2: Move styles to a separate CSS module that doesn't rely on runtime JavaScript.

---

5-7: Consider adding a loading state for the dynamic Chart import.

The Chart component is dynamically imported with ssr: false, which means it only renders on the client. While the import is typically fast, there's a brief moment where nothing is displayed. Adding a loading fallback improves perceived performance.

Apply this diff:

 const Chart = dynamic(() => import('react-apexcharts'), {
   ssr: false,
+  loading: () => (
+    <div className="flex h-[200px] items-center justify-center text-sm text-gray-500">
+      Loading chart...
+    </div>
+  ),
 })

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.

Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.

frontend/__tests__/unit/components/ContributionStats.test.tsx (4)

73-82: Clarify or strengthen icon verification.

The comment on line 79 states "Verify specific icon data attributes" but the following assertions only check that the container and title render, not the actual icon attributes. Icon verification is more thoroughly covered in lines 324-342.

Consider either removing the misleading comment or adding actual icon attribute checks:

-      // Verify specific icon data attributes
-      expect(screen.getByTestId('contribution-stats')).toBeInTheDocument()
-      expect(screen.getByText('Test Contribution Activity')).toBeInTheDocument()
+      // Verify icons are present
+      const chartIcon = icons.find(icon => icon.getAttribute('data-icon') === 'chart-line')
+      expect(chartIcon).toBeInTheDocument()

---

169-181: Strengthen negative value verification.

The test verifies the component doesn't crash with negative values but doesn't assert what's actually displayed. Per the component's formatNumber logic, negative numbers are formatted using toLocaleString() and will display with minus signs.

Add assertions to verify the displayed values:

 render(<ContributionStats title="Negative Stats" stats={negativeStats} />)

-      // Component should still render, showing the negative values or handling them gracefully
 expect(screen.getByText('Negative Stats')).toBeInTheDocument()
+      expect(screen.getByText('-5')).toBeInTheDocument()
+      expect(screen.getByText('-3')).toBeInTheDocument()
+      expect(screen.getByText('-2')).toBeInTheDocument()
+      expect(screen.getByText('-10')).toBeInTheDocument()

---

183-195: Strengthen non-numeric value verification.

The test verifies one valid value (total: 42) but doesn't assert how the invalid string/null/undefined values are displayed. Per the component's formatNumber logic, non-numeric values should display as '0'.

Add assertions to verify fallback behavior:

 render(<ContributionStats title="Invalid Stats" stats={invalidStats} />)

 expect(screen.getByText('Invalid Stats')).toBeInTheDocument()
 expect(screen.getByText('42')).toBeInTheDocument() // total should still work
+      expect(screen.getAllByText('0')).toHaveLength(3) // commits, pullRequests, issues should show 0

---

197-209: Verify large number formatting.

The test confirms the component doesn't crash with very large numbers but doesn't verify they're formatted correctly. Per the component's toLocaleString() usage, these should display with locale-appropriate thousand separators.

Add assertions to verify formatted output:

 render(<ContributionStats title="Large Stats" stats={largeStats} />)

 expect(screen.getByText('Large Stats')).toBeInTheDocument()
-      // Should not crash, even with very large numbers
+      // Verify MAX_SAFE_INTEGER is formatted
+      expect(screen.getByText('9,007,199,254,740,991')).toBeInTheDocument()
+      expect(screen.getByText('999,999,999')).toBeInTheDocument()
+      expect(screen.getByText('888,888,888')).toBeInTheDocument()

frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (4)

404-432: Consider verifying behavior for invalid date inputs.

The tests confirm the component doesn't crash with invalid date ranges (end before start) or malformed dates, but don't verify the expected behavior. Consider adding assertions to check if the component handles these cases appropriately (e.g., swapping dates, showing an error, rendering empty).

---

557-562: Consider using a more stable selector for container verification.

Navigating up two parent elements (parentElement?.parentElement) is fragile and will break if the DOM structure changes. Consider adding a test-id to the container element or using closest() with a class/attribute selector.

Example refactor:

-      const container = screen.getByTestId('contribution-heatmap-chart').parentElement?.parentElement
+      const container = screen.getByTestId('contribution-heatmap-chart').closest('.max-w-5xl')
       expect(container).toHaveClass('max-w-5xl')

---

588-627: Consider verifying error handling behavior.

These tests confirm the component doesn't crash with negative or non-numeric values, but don't verify how these edge cases are handled (e.g., treated as 0, filtered out, displayed as-is). For more thorough coverage, consider adding assertions on the expected behavior.

---

640-671: Performance tests don't verify memoization.

These tests claim to verify memoization but only check that re-renders don't break the component. They don't actually validate that useMemo or useCallback prevent recomputation. To truly test memoization, you'd need to spy on the memoized functions or use React.profiler to count renders. Consider renaming these tests to reflect what they actually verify (e.g., "handles re-renders without issues") or enhancing them with proper memoization assertions.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.

Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.